-
Notifications
You must be signed in to change notification settings - Fork 791
Use fallthrough in optimizeInstructions to further optimize (unsigned)x >= 0 ==> i32(0) #7557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks right here, but see the comment in the test - it is good to annotate tests to make it easy to follow things.
Though, separately, in discussion with @tlively and @osa1 the idea came up to perhaps handle this family of problems in a general way. We do have the Flatten pass for this, which has 2 current downsides:
(i32.ge_u
(i32.load
(i32.const 0)
)
(block (result i32)
(i32.store
(i32.const 0)
(i32.const 0)
)
(i32.const 0)
)
)
That is, I am suggesting that we add a new pass which basically runs Thoughts? |
Does it cost too much to add a new pass which basically runs For my experience on this family of bugs, So maybe we can just request for the help of |
Perhaps, we would need to measure it. But adding another function pass that works in linear time is generally low in cost (all function passes run on a function in sequence, keeping the function in cache, which makes it very fast, unlike global passes that scan the entire wasm - adding a single global pass is usually costly). (Also, I wonder if adding such a pass could allow us to remove
I think there may be many such key points, though. This PR is an example, and many other similar rules could use this, in order to not need the manual fallthrough fix. |
I'll find time next week (holiday weekend starts soon) to experiment with that ChildLocalizer pass. If you're interested to do it though @xuruiyang2002 , feel free to (maybe just comment here if so, so we don't do duplicate work). |
Okay, looking forward to your experiments. |
Currently,
wasm-opt
cannot optimize the code as expected:(unsigned)x >= 0 ==> i32(0)
.It should have been optimized after
merge-blocks
, as thestore
could have been hoisted. However, the load and store cannot be reordered, preventingwasm-opt
from doing so.This can be addressed in
optimizeInstructions
by usingfallthrough
to hoist the constant zero, enabling further optimizations.Fixes: #7556